Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Abstract away the parameters into a parameters class #16

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mark-boer
Copy link

@mark-boer mark-boer commented Jun 14, 2023

Pro's of a parameter class:

  • It is clear what parameters are valid parameters and which are not.
  • We can check for invalid values early (while constructing the parameter class)
  • Mispelling a parameter will lead to a validation error.
  • All the default values are together in one place. Avoiding multiple default values.
  • Better type hints in your editor.
  • The parameters can easily be exported back to a dictionary is neccesary.

Dependencies
For now I used the builtin dataclass object. But we could switch to Pydantic / attrs that automatically bring some validation and some other features.

Discussion
I haven't looked at the interaction with the Teuchos parameterlist. I should check how this interacts.

@Sbte are there other libraries that depend on the old spelling of the parameters? Or is it just Teuchos?

We could also add some conditional checks. For some problem spaces, some parameters do not make sence. It would be nice that those parameters are then also not possible to be set.

TODO

  • all examples still need to be fixed
  • set_parameter does not work yet.

add github's default .gitignore file for python projects (with .python-version)
@Sbte Sbte self-requested a review June 14, 2023 09:47
@Sbte
Copy link
Contributor

Sbte commented Jun 15, 2023

Potential problems I came up with a while ago:

  • Specifying the continuation parameter (string passed to the continuation function) may become really weird.
  • Teuchos expects strings. We could write a conversion function for this.
  • The current parameter list is a mess, because it contains the parameters for different classes. We probably want separate parameter lists for each class. And specify them in the same file as the class.
  • If we do the above, specifying the parameters in the actual run scripts (examples) may become a mess, because you need to import 4 different parameter classes.

@mark-boer
Copy link
Author

mark-boer commented Jun 15, 2023

Specifying the continuation parameter (string passed to the continuation function) may become really weird.

You could simply use the same parameter names such as param_name="rayleigh_number" and then use getattr(parameters, param_name) or use a translation table (similar to Teuchos) and add a function to parameters def get_param_by_full_name(self, param_name) or something. There is plenty of options :-)

The current parameter list is a mess, because it contains the parameters for different classes. We probably want separate parameter lists for each class. And specify them in the same file as the class.
If we do the above, specifying the parameters in the actual run scripts (examples) may become a mess, because you need to import 4 different parameter classes.

You can compose a parameters class made up of multiple sub-classes, in the same way that is now already being done with the EigenValueSettings which is one of the attributes of the Parameters class. If you do not want to import multiple subclasses you can do it without:

option 1, create the default parameters and override options:

    # Define the problem
    parameters = Parameters(
        problem_type='Differentially Heated Cavity',
    )
    parameters.eigenvalue_solver.target = 0
    parameters.eigenvalue_solver.tolerance = 1e-9
    parameters.eigenvalue_solver.number_of_eigenvalues = 5

option 2: have it parse a dictionary

    parameter = Parameters.from_dict({
        'problem_type': 'Differentially Heated Cavity',
        'eigenvalu_solver': {
            'target': 0,
            'tolerance': 1e-9,
        },
    })

@Sbte
Copy link
Contributor

Sbte commented Jun 15, 2023

Oh, another issue I came up with is that external models (not from Discretization.py) also use e.g. Continuation.py and the eigenvalue solver interface. They have a huge range of parameters. See for instance the list here:

https://github.com/nlesc-smcm/pop-iemic-examples/blob/main/iemic.py#L79

We want to keep that use case working as well.

@dajuno
Copy link
Collaborator

dajuno commented Oct 18, 2024

I'm not sure which of the arguments above still apply to the current code, nearly 1.5 years after your discussion.

My impression is that with !42 !43, the main obstacle to parameters (data)classes has been removed. Continuation is unaware of a parameter dict and lets the interface handle parameter updates. (Not sure if this was the intentation of that change?) It seems that iemic also uses classes for their parameters.

Any discretization & interface implementing parameter dictionaries or dataclasses must just implememt get/set_parameter methods, in the latter case with getattr/setattr as @mark-boer suggested.

If the code must be compatible with Python 3.6 (i.e., no dataclasses), I think normal classes are still preferable to dictionaries.

What are your thoughts on this now?

@LourensVeen
Copy link
Collaborator

I don't have the design clear here nor the time to dive into it, but in general everything Mark said is right. Classes are self-documenting in that at least you know which parameters there are, and with type annotations and a type checker you can find many issues much more easily. And as Mark also pointed out, Python has plenty introspection to be able to convert between things as needed. (The iemic example linked above actually shows how OMUSE uses a class-based interface, which has getattr behind the scenes to forward those parameter set operations to the underlying native code.)

As for Python 3.6, I'm pretty conservative with what I support, but the Python community tends to limit support for new releases to Python versions still in official support. Since many packages on PyPI have broken metadata, installing anything on a no-longer-supported version gets difficult pretty quickly, which incentivises people to upgrade. So even though e.g. Ubuntu 18.04 has Python 3.6 and has (paid) support until 2028, anyone still running that OS has already installed a newer Python on it by now. The upshot is that as of last week, you can take 3.9 as a minimum for new releases without upsetting anyone.

@mark-boer
Copy link
Author

I see a lot of the issues have already been solved by explicitly listing the parameters in the constructors, instead of accepting an arbitrary dictionary.

If you wish to have an equivalent of dataclasses, that works on Python3.6, you could consider attrs (it's also better than regular dataclasses).

Feel free to close this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants